msglist: Distinguish `isBot: true` message senders with a bot marker by sm-sayedi · Pull Request #605 · zulip/zulip-flutter

sm-sayedi · 2024-03-29T21:02:01Z

A bot icon is shown with the name of the bot users.

Before	After

Fixes: #156

sm-sayedi · 2024-03-29T21:34:31Z

In the test before this test, I noticed that there was a fake HttpClient set, that could be replaced by calling this method that was factored out in #590.

testWidgets('Updates avatar on RealmUserUpdateEvent', (tester) async { - final httpClient = FakeImageHttpClient(); - debugNetworkImageHttpClientProvider = () => httpClient; - httpClient.request.response - ..statusCode = HttpStatus.ok - ..content = kSolidBlueAvatar; + prepareBoringImageHttpClient(); }

Is it ok if I add an additional [NFC] commit to this PR with the requested change, or should we just ignore it for now?

Seems reasonable to me! 🙂

chrisbobbe

Thanks, this looks mostly good! Small comments below.

chrisbobbe · 2024-03-29T23:15:47Z

Let's match the color to what the web app uses.

I'm also curious how the 18px size is chosen, and the 8px distance from the user's name. To my eye I think those values might both want to be a bit smaller. What does the web app do?

This should be fixed. Even on the webapp the bot icon isn't super prominent, and I have no idea why it's blurred.

I saw this comment on CZO, so I decided to go with a more prominent icon. Any recommendations from your side are most welcome!

On the web, the icon size is 12px, the color is rgb(159, 173, 173), and the distance to the user's name is 5px.
With the same properties applied, it looks like this:

Should I go with this?

Hmm. To me, that all looks good except the size; I think it looks too small now. The font size in the web app is smaller than in the mobile app, so maybe this icon should be scaled up according to that. Maybe try 15px?

Applied the changes. Updated the description for the new images. Please have a look!

chrisbobbe · 2024-03-29T23:16:21Z

Seems reasonable to me! 🙂

chrisbobbe · 2024-03-29T23:17:07Z

Can you check specifically that the icon appears next to the sender that is a bot, and also that an icon doesn't appear on the senders that are not bots?

Sure thing!

sm-sayedi · 2024-03-30T00:04:30Z

Thanks for the review! A few comments above.

gnprice

Thanks @sm-sayedi for handling this, and @chrisbobbe for the review!

Other than the things Chris remarked on above, this all looks good to me, except a few nits below.

gnprice · 2024-04-01T03:30:22Z

This is probably clearer as a list literal with three explicit eg.user calls: [eg.user(isBot: true), eg.user(isBot: false), etc. Not much longer that way, and less logic for the reader to unpack.

gnprice · 2024-04-01T03:39:27Z

nit: (two nits)

Suggested change

final message = item.message;

final messageSender =

PerAccountStoreWidget.of(context).users[message.senderId];

final store = PerAccountStoreWidget.of(context);

final message = item.message;

final sender = store.users[message.senderId];

That way the result of PerAccountStoreWidget.of goes into a store local, like in most of our other build methods where it appears. The uniformity is mildly helpful; and this way also makes it maximally easy to add another bit of logic using the store later, without either duplicating the lookup or rewriting these lines.

Separately, most everything within this method is about the particular message message, so a name "sender" communicates basically the same meaning as "message sender". Similarly the existing other locals include senderRow and time, without the added qualifier "message".

gnprice · 2024-04-01T03:44:50Z

Oh and one other nit, in the commit message:

ui: Distinguish `isBot: true` users with a bot marker

Fixes: #156

Let's make the prefix msglist: — that's more specific than "ui", and lets the reader know this is about how we show users in specifically the message list.

Similarly, s/users/message senders/ would further help pin down where in the UI we're talking about (and the resulting summary line is 68 columns, so there's room; summary lines can go a bit over 70).

sm-sayedi · 2024-04-01T09:02:34Z

Thanks for the review! Pushed the latest revision. PTAL!

sm-sayedi · 2024-04-01T09:04:09Z

Similarly, s/users/message senders/ would further help pin down where in the UI we're talking about (and the resulting summary line is 68 columns, so there's room; summary lines can go a bit over 70).

Sorry, I am not sure where to add this in the commit message!

sm-sayedi · 2024-04-01T10:00:13Z

For a bot user, this evaluates to two results, and that's because there are two Rows that satisfy this finder; one direct parent Row and another, the parent of this parent Row, both are for the same user. To make sure they're for the same user, the following two lines are added just before the finder:

check(nameFinder.evaluate().singleOrNull).isNotNull(); check(botFinder.evaluate().singleOrNull).isNotNull();

This makes sure the fullName text for a user is only shown once on the screen. The bot icon is also checked for being shown just once, but that does not guarantee it belongs to the current user. That check is done by check(userFinder.evaluate()).isNotEmpty().

I hope this test is fine for checking if a bot icon is shown next to a user name.

gnprice · 2024-04-01T17:31:48Z

Similarly, s/users/message senders/ would further help pin down where in the UI we're talking about (and the resulting summary line is 68 columns, so there's room; summary lines can go a bit over 70).

Sorry, I am not sure where to add this in the commit message!

Ah, that was perhaps a bit cryptic of a way to put it. What I meant by "s/users/message senders/" was "replace 'users' with 'message senders'".

(The s/// syntax is found in various venerable tools from the Unix world. Here's Perl's documentation for it.)

sm-sayedi · 2024-04-01T18:19:51Z

Ah, that was perhaps a bit cryptic of a way to put it. What I meant by "s/users/message senders/" was "replace 'users' with 'message senders'".

(The s/// syntax is found in various venerable tools from the Unix world. Here's Perl's documentation for it.)

Thanks, just learned something new! Revision pushed!

gnprice · 2024-04-03T22:42:30Z

Thanks for the revision!

This all looks good to me except one more commit-message nit:
23b1fe1d0 msglist-test [nfc]: Refactor some code to use prepareBoringImageHttpClient
f2cf926db msglist: Distinguish isBot: true message senders with a bot marker

For that first commit, the prefix should be msglist test [nfc]:, with a space, rather than msglist-test [nfc]:. In general if you look at git log --oneline, you'll see that the "test" modifier is always separated by a space.

Other than that I'll leave this for @chrisbobbe to re-review for the icon size and placement, and anything else he notices, and then merge.

sm-sayedi · 2024-04-04T02:08:52Z

Thanks for the review! Revision pushed.

Now waiting for a review from your side @chrisbobbe!

…Client` In the test ('Updates avatar on RealmUserUpdateEvent'), the code for setting a `FakeImageHttpClient` is replaced by the `prepareBoringImageHttpClient` function.

Fixes: zulip#156

chrisbobbe · 2024-04-09T20:05:10Z

Thanks, looks good to me! Merging.

After this, there are still some places in the UI where we show the name of a bot user without making it clear it's a bot. For example, it would probably be helpful to show the bot icon next to a bot user's name on the profile page (ProfilePage). I'm not sure if we need to show the icon in all the places where names appear, though. Would you be willing to file a new issue with a checklist of all the places where bots' names might be displayed in the app, indicating which ones should start marking bots with the bot icon, which ones don't need to do that, and which ones you're not sure about?

sm-sayedi · 2024-04-16T20:10:55Z

Would you be willing to file a new issue with a checklist of all the places where...

Sure, will do so.

sm-sayedi marked this pull request as ready for review March 29, 2024 21:02

sm-sayedi commented Mar 29, 2024

View reviewed changes

chrisbobbe reviewed Mar 29, 2024

View reviewed changes

gnprice reviewed Apr 1, 2024

View reviewed changes

sm-sayedi force-pushed the issue-156-distinguish-bot-users branch 4 times, most recently from 14697c4 to 2570ae4 Compare April 1, 2024 08:46

sm-sayedi force-pushed the issue-156-distinguish-bot-users branch from 2570ae4 to e0db6c1 Compare April 1, 2024 09:42

sm-sayedi commented Apr 1, 2024

View reviewed changes

sm-sayedi changed the title ~~ui: Distinguish isBot: true users with a bot marker~~ msglist: Distinguish isBot: true users with a bot marker Apr 1, 2024

sm-sayedi force-pushed the issue-156-distinguish-bot-users branch from e0db6c1 to 85051a3 Compare April 1, 2024 18:12

sm-sayedi changed the title ~~msglist: Distinguish isBot: true users with a bot marker~~ msglist: Distinguish isBot: true message senders with a bot marker Apr 1, 2024

sm-sayedi force-pushed the issue-156-distinguish-bot-users branch 2 times, most recently from 8038a39 to f2cf926 Compare April 3, 2024 01:32

sm-sayedi requested a review from gnprice April 3, 2024 01:38

sm-sayedi force-pushed the issue-156-distinguish-bot-users branch from f2cf926 to 50d079c Compare April 3, 2024 23:58

sm-sayedi requested a review from chrisbobbe April 4, 2024 00:03

sm-sayedi added 2 commits April 8, 2024 06:34

msglist test [nfc]: Refactor some code to use `prepareBoringImageHttp…

5c47233

…Client` In the test ('Updates avatar on RealmUserUpdateEvent'), the code for setting a `FakeImageHttpClient` is replaced by the `prepareBoringImageHttpClient` function.

msglist: Distinguish isBot: true message senders with a bot marker

8c0f9ae

Fixes: zulip#156

sm-sayedi force-pushed the issue-156-distinguish-bot-users branch from 50d079c to 8c0f9ae Compare April 8, 2024 02:05

chrisbobbe merged commit 8c0f9ae into zulip:main Apr 9, 2024

chrisbobbe mentioned this pull request Apr 26, 2024

ui: Distinguish isBot: true users in more places, not just message senders in the message list #636

Open

sm-sayedi mentioned this pull request Jun 26, 2024

ui: Distinguish bot users in more places #764

Closed

-    final message = item.message;
-    final messageSender =
-      PerAccountStoreWidget.of(context).users[message.senderId];
+    final store = PerAccountStoreWidget.of(context);
+    final message = item.message;
+    final sender = store.users[message.senderId];

Conversation

sm-sayedi commented Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm-sayedi Mar 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm-sayedi commented Mar 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Apr 1, 2024

Uh oh!

sm-sayedi commented Apr 1, 2024

Uh oh!

sm-sayedi commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Apr 1, 2024

Uh oh!

sm-sayedi commented Apr 1, 2024

Uh oh!

gnprice commented Apr 3, 2024

Uh oh!

sm-sayedi commented Apr 4, 2024

Uh oh!

chrisbobbe commented Apr 9, 2024

Uh oh!

sm-sayedi commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sm-sayedi commented Mar 29, 2024 •

edited

Loading

sm-sayedi Mar 30, 2024 •

edited

Loading

sm-sayedi commented Mar 30, 2024 •

edited

Loading

sm-sayedi commented Apr 1, 2024 •

edited

Loading